Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add helpful error for renamed Wasm module #799

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

PgBiel
Copy link
Contributor

@PgBiel PgBiel commented Jul 14, 2024

An initial countermeasure for the problem at #438 (comment) - we just throw an error if ${crate_name_with_underscores}.wasm doesn't exist when it should. The error message needs some improvement, but at least I could test this approach and it produces the result below.

image

Right now, I just throw an error with a summarized description of the problem and print a longer error to console. But I think there's some improvement to be had here, I'm open for suggestions. Maybe we can just throw the error with the full message anyway, or we can append "Check the browser console for more details" to the error message.

@Bromeon Bromeon added quality-of-life No new functionality, but improves ergonomics/internals c: wasm WebAssembly export target labels Jul 14, 2024
@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-799

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, these kind of improvements are very appreciated! 💪

@@ -54,6 +54,11 @@ pub fn attribute_gdextension(item: venial::Item) -> ParseResult<TokenStream> {
let script = std::ffi::CString::new(concat!(
"var pkgName = '", env!("CARGO_PKG_NAME"), "';", r#"
var libName = pkgName.replaceAll('-', '_') + '.wasm';
if (!(libName in LDSO.loadedLibsByName)) {
// Always print to console, avoid problems with automatic error suppression.
console.error(`gdext could not find the Wasm module '${libName}', needed to load the '${pkgName}' crate's GDExtension. Please ensure a file named '${libName}' exists in the game's web export files.`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needed to load the '${pkgName}' crate's GDExtension.

Does the "GDExtension" part really add something, or could this be simplified to

needed to load the '${pkgName}' crate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also maybe you can call it "godot-rust" instead of "gdext". This could be useful in case someone uses multiple languages or extensions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree! It looks like this now:

image

@PgBiel PgBiel force-pushed the error-renamed-wasm branch from 4102319 to afd47f1 Compare July 15, 2024 01:22
@Bromeon Bromeon added this pull request to the merge queue Jul 17, 2024
Merged via the queue into godot-rust:master with commit b30c2f2 Jul 17, 2024
15 checks passed
@Bromeon
Copy link
Member

Bromeon commented Jul 17, 2024

Thank you! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: wasm WebAssembly export target quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants